[Stalled] Provide an Elsa class that can be used programmatically#39
[Stalled] Provide an Elsa class that can be used programmatically#39hroncok wants to merge 3 commits intopyvec:masterfrom
Conversation
elsa/_cli.py
Outdated
|
|
||
| Arguments: | ||
|
|
||
| * app: and instance of Flask WSGI application |
There was a problem hiding this comment.
*an instance of a Flask WSGI application
(typo and missing article)
elsa/_cli.py
Outdated
| base_url = (base_url or self.base_url or | ||
| self.app.config.get('FREEZER_BASE_URL')) | ||
| if not base_url: | ||
| raise ValueError('No base URL provided!') |
There was a problem hiding this comment.
I think errors don't use punctuation by convention
| warnings.filterwarnings('error', | ||
| category=flask_frozen.FrozenFlaskWarning) | ||
|
|
||
| print('Generating HTML...') |
There was a problem hiding this comment.
Should an API like this print on its own? Perhaps logging as INFO would be better, since a logger is already in use. Alternatively, a verbose parameter.
There was a problem hiding this comment.
Nope, it definitively should not, I say that in the PR description. A logger is already in use?
There was a problem hiding this comment.
I missed that, sorry. Also, I didn't realize warnings is not directly related to logging. My mistake.
| print('Generating HTML...') | ||
| self.freezer.freeze() | ||
|
|
||
| def freeze_fail_exit(self, path=None, base_url=None): |
There was a problem hiding this comment.
Is this useful? Usually libraries don't quit on their own.
Could it be collapsed into a parameter of freeze (like exit_on_failure)?
There was a problem hiding this comment.
I need to do it twice, so I created a function. Maybe it should be underscored, not to be part of the API.
There was a problem hiding this comment.
I can only see you using it once right now. If it's only used in the CLI, I'd say it could be defined in there, but your CLI is intertwined with the Elsa class anyway (probably not really a bad thing).
Underscore should be fine too.
There was a problem hiding this comment.
I can only see you using it once right now
Good catch. Now I use it twice :D
|
|
||
| def serve(self, port=DEFAULT_PORT): | ||
| """Serve the frozen app using the freezers ability to serve what was | ||
| frozen""" |
|
Should the |
|
I'm honestly not sure. I probably would have put it outside myself, but I don't think the current state of things is wrong. |
|
OK, let me work on this as is, will try to fix all the printing and error reporting and change it so that only the actual CLI is talking. Will also try to add some unittests, since now we are defining an API. |
|
Sounds good to me! |
|
BTW I was thinking about this more and I think making cli() a separate function once again and just create an Elsa instance on each command will be more maintainable. |
hroncok
changed the title
hroncok
dismissed
Sanqui’s stale review
I want to block this somehow, yet I cannot add my own "change required" review
2 participants
Related: #32
This is still very much work in progress, currently:
freeze(),serve()anddeploy()cli(...)is now a shortcut toElsa(...).cli()However:
I'd like an early feedback if possible.